Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix to_bytes in fips204.py #1

Merged
merged 1 commit into from
Sep 13, 2024
Merged

Fix to_bytes in fips204.py #1

merged 1 commit into from
Sep 13, 2024

Conversation

mlindgren
Copy link
Contributor

fips204.py uses Python's built-in int.to_bytes(...) method, but does not provide the required arguments length and byteorder. This results in an exception:

Traceback (most recent call last):
  File "./fips204.py", line 904, in <module>
    test_mldsa( ml_dsa.keygen_internal,
  File "/home/mitch/py-acvp-pqc/test_mldsa.py", line 221, in test_mldsa
    fail += mldsa_test_keygen(keygen_kat, keygen_func, iut)
  File "/home/mitch/py-acvp-pqc/test_mldsa.py", line 45, in mldsa_test_keygen
    (pk, sk)    = keygen_func(  bytes.fromhex(x['seed']),
  File "./fips204.py", line 177, in keygen_internal
    se = self.h(xi + self.k.to_bytes() + self.ell.to_bytes(), 128)
TypeError: to_bytes() missing required argument 'length' (pos 1)

This change fixes the code to use the integer_to_bytes method that is defined as part of the ML_DSA class.

Note that the same issue exists in fips203.py, but I didn't fix that one because I'm less familiar with that algorithm.

@mjosaarinen
Copy link
Owner

Yes, probably a good idea to be explicit about length there. Out of curiosity -- which version of Python you were using & do you know which versions of Python have this as mandatory (when the integer is <128) ?

@mlindgren
Copy link
Contributor Author

I'm on 3.8 (I was using an old Ubuntu 20.04 VM). Looks like the defaults were added in 3.11.

@mjosaarinen mjosaarinen merged commit 0382529 into mjosaarinen:main Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mlindgren @mjosaarinen